Skip to content

Ryan/remove axios#137

Open
RyanLCox1 wants to merge 16 commits intomasterfrom
ryan/remove_axios
Open

Ryan/remove axios#137
RyanLCox1 wants to merge 16 commits intomasterfrom
ryan/remove_axios

Conversation

@RyanLCox1
Copy link
Copy Markdown
Contributor

Get rid of Axios dependency.

Removes axios and axios-retry dependencies in response to the axios
npm supply chain compromise. HTTP requests now use the native fetch API
(Node 18+ / browsers) with undici's ProxyAgent for proxy support.

- Rewrote HttpSender to use fetch with AbortSignal.timeout
- Added ProxyConfig interface to types.ts (replaces AxiosProxyConfig)
- Updated ClientBuilder.withProxy() to build a proxy URL string
- Renamed AxiosLikeResponse to HttpResponse in buildSmartyResponse.ts
- Added injectable fetchFn parameter for unit test mocking
- Added integration tests using a local HTTP server
…ging

- Add withProxy(url) single-arg overload for simpler proxy configuration
- Narrow header types from Record<string, unknown/any> to Record<string, string>
- Add debug logging for JSON parse failures in HttpSender
- Add comment explaining undici dispatcher type bypass
- Declare Node >=18.0.0 engine requirement (fetch is built-in from v18)
- Remove silent swallow of JSON parse errors in HttpSender
- Narrow buildSmartyResponse error field from `any` to `string | Error`
- Widen Response.error to `Error | string | null` to match actual usage
- Make MockSenderWithStatusCodesAndHeaders.send() return Promise<Response>
- Remove CompatibleMockSender wrapper and mock interfaces from types.ts
… tighten types

RetrySender now catches rejected responses from the sender chain and
converts them back to return values for retry evaluation. Mock sender
updated to throw on >= 400 to match real HttpSender behavior. Removed
ProxyConfig interface in favor of inline type, typed dispatcher as
undici.Dispatcher, and cleaned up let -> const in retry tests.
Narrow Response.error back to Error | null (was Error | string | null)
by wrapping string errors in buildSmartyResponse's new normalizeError
helper. Validate baseUrl is an absolute HTTP(S) URL before constructing
a URL object. Safely wrap non-Error throwables in the fetch catch block.
@RyanLCox1 RyanLCox1 requested a review from smartymanwill April 2, 2026 18:35
@camiblanch camiblanch self-requested a review April 2, 2026 19:49
package.json Outdated
},
"author": "Smarty SDK Team <support@smarty.com> (https://www.smarty.com)",
"engines": {
"node": ">=18.0.0"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be updated to 20 instead of 18? That's the minimum version we have in the github workflow that runs the tests

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll bump it to 20

const { ProxyAgent } = require("undici");
this.dispatcher = new ProxyAgent(config.url);
} catch {
throw new Error(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I pass an invalid URL to my proxy (for example, a URL with a path like https://us-street.api.smarty.com/street) then I just see this error. I think it would be better to have more specific error handling here if possible so I know what's actually wrong if I use a bad URL

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed (and tested myself!)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how much it matters to you, but this file isn't "prettiered."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prettiered.

private initProxy(config: { url: string }): void {
try {
// eslint-disable-next-line @typescript-eslint/no-require-imports
const { ProxyAgent } = require("undici");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am running us_street.ts but pointed to "../dist/esm/index.mjs" instead of "smartystreets-javascript-sdk" to test the changes you have made and when I run it with .withProxy("https://us-street.api.smarty.com"); on my clientBuilder (which doesn't actually proxy, but it forces the code to use the proxy code) I get r [Error]: unexpected error

I think instead if you change it to use an awaited import (and update the method to be async), then it works! Then you don't need the eslint disable either

Suggested change
const { ProxyAgent } = require("undici");
const { ProxyAgent } = await import("undici");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

…rrors, use dynamic import

- Preserve HTTP status codes when parseResponseBody throws (e.g. malformed
  JSON on a 4xx/5xx response) instead of defaulting to status 0
- Split proxy init into two try/catch blocks so invalid proxy URLs get a
  clear message instead of the misleading "install undici" error
- Switch from require() to await import() for undici so proxy works in
  ESM contexts; store promise and await in send()
try {
return await this.inner.send(request);
} catch (error) {
if (error && typeof error === "object" && "statusCode" in error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a slight behavioral change here. trySend() converts thrown error responses into resolved return values instead of throwing them (as it did previously). For example, with sender.send(request).catch(handleError), if retry attempts are exhausted for a 500-level response, the code may silently receive an error response through the success path. Is that change intentional?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a way. After retry is exhausted, we will return the error.

if (username && password) {
auth = `${encodeURIComponent(username)}:${encodeURIComponent(password)}@`;
}
this.proxy = { url: `${protocol}://${auth}${hostOrUrl}:${port}` };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor, but for plain JS implementations (without TS checking), if protocol is undefined at runtime, this produces undefined://host:port which will silently pass through and fail later in ProxyAgent (the Axios code stored it as a plain object so this wasn't an issue.). Something like if (!protocol) throw new Error(...) could catch this early.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

…unknown errors

- Handle statusCode 0 (network errors) explicitly, keeping the original
  error (e.g. "fetch failed") instead of overwriting with "unexpected error"
- Fall back to the existing error message in the default case when the
  response payload has no API error details
trySend() intentionally converts thrown error responses to return values
so the retry loop can inspect statusCode and decide whether to retry.
But after the loop, error responses were silently returned through the
success path. Now re-throws if the response has an error or still carries
a retryable status code.
Without this guard, calling withProxy(host, port) without a protocol
from plain JS produces "undefined://host:port" which silently passes
through and fails later in ProxyAgent with a confusing error.
Replace the compound check (response.error || statusToRetry.includes)
with a straightforward status code check: throw if the response is not
a success. This also fixes non-retryable errors like 422 that were
silently returning through the success path.
Copy link
Copy Markdown
Contributor

@smartymanwill smartymanwill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants